Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deploying by a contract name [1/2] — refactor feature dependencies #565

Merged
merged 21 commits into from
Aug 4, 2022

Conversation

kasperski95
Copy link
Contributor

@kasperski95 kasperski95 commented Aug 2, 2022

  • refactored ProjectCompiler
  • moved project_compiler.py out of commands/build
  • extracted ProjectCairoPathBuilder
  • extracted ContractWriter
  • changed ProjectCompiler methods to be smaller and more specific

@kasperski95 kasperski95 self-assigned this Aug 2, 2022
@codecov
Copy link

codecov bot commented Aug 2, 2022

Codecov Report

Merging #565 (33ad368) into master (7ac3714) will increase coverage by 0.00%.
The diff coverage is 89.37%.

@@           Coverage Diff           @@
##           master     #565   +/-   ##
=======================================
  Coverage   82.44%   82.45%           
=======================================
  Files         175      177    +2     
  Lines        5943     6029   +86     
=======================================
+ Hits         4900     4971   +71     
- Misses       1043     1058   +15     
Impacted Files Coverage Δ
.../commands/init/project_creator/_project_creator.py 81.81% <ø> (ø)
protostar/commands/install/install_command.py 70.45% <0.00%> (ø)
protostar/commands/remove/remove_command.py 72.50% <ø> (ø)
protostar/commands/test/expected_event.py 100.00% <ø> (ø)
protostar/commands/update/update_command.py 61.22% <0.00%> (ø)
protostar/commands/build/build_command.py 76.66% <66.66%> (-0.76%) ⬇️
...tostar/protostar_toml/protostar_project_section.py 90.47% <83.33%> (-3.47%) ⬇️
...star/protostar_toml/protostar_contracts_section.py 93.87% <84.61%> (-3.35%) ⬇️
protostar/compiler/project_compiler.py 89.55% <89.55%> (ø)
protostar/compiler/project_cairo_path_builder.py 91.66% <91.66%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kasperski95 kasperski95 changed the title Support deploying and declaring by a contract name Deploying by a contract name — Refactor dependencies Aug 3, 2022
@kasperski95 kasperski95 changed the title Deploying by a contract name — Refactor dependencies Deploying by a contract name — refactor dependencies Aug 3, 2022
@kasperski95 kasperski95 changed the title Deploying by a contract name — refactor dependencies Deploying by a contract name — refactor feature dependencies Aug 3, 2022
@kasperski95 kasperski95 marked this pull request as ready for review August 3, 2022 10:50
@kasperski95 kasperski95 changed the title Deploying by a contract name — refactor feature dependencies Deploying by a contract name [1/2] — refactor feature dependencies Aug 3, 2022
protostar/protostar_toml/protostar_contracts_section.py Outdated Show resolved Hide resolved
from starkware.starknet.services.api.contract_class import ContractClass


class CompiledContractWriter:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be a plain function

Copy link
Contributor Author

@kasperski95 kasperski95 Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, it could. Something feels off when mixing paradigms.

It's easier to find a file that contains a class rather than a list of functions. Naming files in the same way as the main exported function is confusing because a function and a filename use the same casing, and that makes importing/patching confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still define single public module function, and name the module the same way. That's what I have seen in the wild.

Something feels off when mixing paradigms.

To me, using classes too much in Python/JS is mixing paradigms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still define single public module function, and name the module the same way. That's what I have seen in the wild.

I did it when I used the functional paradigm. Auto import functionality confused the function with the module, if they were named in the same way. Also, I couldn't patch the function, if it was imported from the __init__.py (barrel file) from the same reason.

To me, using classes too much in Python/JS is mixing paradigms.

Functional programming is more common in Python, but it doesn't mean that this is the proper tool for the job.

  • Object-Oriented Programming is the tool best suited for defining how we cross architectural boundaries with polymorphism and plugins
  • Functional programming is the tool we use to push data to the boundaries of our applications
  • and Structured programming is the tool we use to write algorithms

https://khalilstemmler.com/articles/software-design-architecture/full-stack-software-design/

protostar/compiler/compiled_contract_writer.py Outdated Show resolved Hide resolved
protostar/compiler/compiled_contract_writer.py Outdated Show resolved Hide resolved
protostar/compiler/__init__.py Outdated Show resolved Hide resolved
protostar/commands/build/build_command.py Outdated Show resolved Hide resolved
protostar/compiler/project_compiler.py Outdated Show resolved Hide resolved
@kasperski95 kasperski95 requested a review from mkaput August 3, 2022 13:49
@kasperski95 kasperski95 merged commit d7c9020 into master Aug 4, 2022
@kasperski95 kasperski95 deleted the feature/support-contract-names-in-cheatcodes branch August 4, 2022 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploy from migration cheatcodes by contract name specified in protostar.toml
2 participants